Add AllocationTrackedMemoryManager and refactor allocators#3120
Conversation
JimBobSquarePants
added
breaking
|
It would be very helpful for my review if we could revert #3056, and rebase this on top of the original state. I want to review the feature implementation as a whole thing and now it's difficult to react to specific changes from that PR. |
JimBobSquarePants
force-pushed
the
js/optimize-allocation-tracking
branch
from
127b5bb to
15fd941
Compare
|
#3056 has been reverted and this PR rebased over the top. Hopefully this is much easier to review now the diff is clean. |
|
Thanks a lot! I'll come back to this once the Drawing API is concluded. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the MemoryAllocator infrastructure to support accumulative (lifetime) allocation tracking without per-allocation wrapper overhead by introducing a new tracked memory-owner base type (AllocationTrackedMemoryManager<T>) and moving allocator implementations to an AllocateCore pattern.
Changes:
- Introduces
AllocationTrackedMemoryManager<T>+AllocationTrackingStateand makesMemoryAllocator.Allocatenon-virtual with tracking handled centrally. - Updates built-in allocators and memory-owner implementations (managed/unmanaged + memory groups) to participate in the new tracking lifecycle.
- Extends tests to validate that accumulative allocation reservations are released on owner/group disposal and stabilizes pool-based tests by disabling trimming.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ImageSharp.Tests/TestUtilities/TestMemoryAllocator.cs | Updates test allocator to implement AllocateCore and return a tracked memory manager. |
| tests/ImageSharp.Tests/Memory/DiscontiguousBuffers/MemoryGroupTests.Allocate.cs | Disables pool trimming in a test to avoid timing-related flakiness. |
| tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs | Adjusts allocator factory typing, adds overflow-safe AllocHGlobal, and adds accumulative-limit release tests. |
| tests/ImageSharp.Tests/Memory/Allocators/SimpleGcMemoryAllocatorTests.cs | Adds accumulative-limit release tests for owner and group disposal. |
| tests/ImageSharp.Tests/Image/ProcessPixelRowsTestBase.cs | Updates mock allocator to implement AllocateCore returning tracked owners. |
| src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs | Releases allocation tracking when owned groups are disposed; minor indexing cleanup. |
| src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs | Adds allocation tracking state to groups and routes group segment allocation through AllocateGroupBuffer. |
| src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.Consumed.cs | Releases allocation tracking when consumed groups are disposed; minor expression-bodied cleanup. |
| src/ImageSharp/Memory/DiscontiguousBuffers/IMemoryGroup{T}.cs | Adds explicit public modifiers on interface members. |
| src/ImageSharp/Memory/Allocators/UnmanagedMemoryAllocator.cs | Migrates to AllocateCore, adds AllocateGroupBuffer, and exposes AllocateBuffer helper for raw unmanaged owners. |
| src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs | Migrates to AllocateCore and uses raw unmanaged allocation for non-pooled fallback. |
| src/ImageSharp/Memory/Allocators/SimpleGcMemoryAllocator.cs | Adds options-based constructor and migrates to AllocateCore. |
| src/ImageSharp/Memory/Allocators/MemoryAllocatorOptions.cs | Adds AccumulativeAllocationLimitMegabytes option with validation and docs. |
| src/ImageSharp/Memory/Allocators/MemoryAllocator.cs | Implements centralized accumulative tracking, suppression scope, and AllocateGroupBuffer hook. |
| src/ImageSharp/Memory/Allocators/Internals/UnmanagedBuffer{T}.cs | Switches unmanaged owner to tracked base type and updates dispose path. |
| src/ImageSharp/Memory/Allocators/Internals/SharedArrayPoolBuffer{T}.cs | Updates disposal to DisposeCore and aligns lifetime guard field semantics. |
| src/ImageSharp/Memory/Allocators/Internals/ManagedBufferBase.cs | Switches managed buffer base to the tracked base type. |
| src/ImageSharp/Memory/Allocators/Internals/BasicArrayBuffer.cs | Updates disposal override to DisposeCore. |
| src/ImageSharp/Memory/Allocators/AllocationOptionsExtensions.cs | Removes BOM and adds XML documentation for the Has helper. |
| src/ImageSharp/Memory/AllocationTrackingState.cs | New: state container to release a single reservation exactly once. |
| src/ImageSharp/Memory/AllocationTrackedMemoryManager{T}.cs | New: tracked MemoryManager<T> base that releases reservations on dispose. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/ImageSharp/Memory/Allocators/MemoryAllocatorOptions.cs:45
AllocationLimitMegabytesvalidates its own value, but it does not validate against an already-setAccumulativeAllocationLimitMegabytes. This means settingAccumulativeAllocationLimitMegabytesfirst and then settingAllocationLimitMegabytesto a larger value can create inconsistent options (the laterApplyOptionswill allow single allocations larger than the accumulative limit). Add a symmetric validation in theAllocationLimitMegabytessetter whenAccumulativeAllocationLimitMegabytes.HasValueto ensureAllocationLimitMegabytes <= AccumulativeAllocationLimitMegabytesregardless of assignment order.
{
if (value.HasValue)
{
Guard.MustBeGreaterThan(value.Value, 0, nameof(this.AllocationLimitMegabytes));
}
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
| /// Initializes a new instance of the <see cref="SimpleGcMemoryAllocator"/> class with custom limits. | ||
| /// </summary> | ||
| /// <param name="options">The <see cref="MemoryAllocatorOptions"/> to apply.</param> | ||
| public SimpleGcMemoryAllocator(MemoryAllocatorOptions options) => this.ApplyOptions(options); |
There was a problem hiding this comment.
@JimBobSquarePants MemoryAllocatorOptions were not meant to be attached to this type (the feature set is mostly orthogonal). Also, I don't think it brings any value to users to use allocation tracking for anything but the default memory allocator. Scoping the tracking feature as an implementation detail of UniformUnmanagedMemoryPoolMemoryAllocator could simplify things significantly.
Sorry, I was extremely distracted last week because of starting my new job, but I REALLY wanted to come back to this. I can do a deeper post-review tomorrow afternoon.
There was a problem hiding this comment.
I noticed the release is out now :( Maybe deleting this ctr and AllocationTrackedMemoryManager<T> from the public APIS is not much of a breaking change for a minor update that refactors this. Those things shouldn't really be exposed.
There was a problem hiding this comment.
I had to ship I’m afraid. No need to apologize at all.
I actually think we should treat SimpleGcMemoryAllocator the same as others here. I'd rather normalize everything through a single channel and ensure that all implementations have tracking.
Prerequisites
Description
An updated implementation of #3056
@antonfirsov your comment in the PR was the key to a better implementation for me.
I was so determined not to make a breaking change that I massively overcomplicated the implementation and made it inefficient.
This PR changes things by implementing a new type
AllocationTrackedMemoryManagerwhich custom allocator memory managers must implement. By makingMemoryAllocator.Allocatenon virtual and adding an abstractAllocateCoreI can introduce tracking without the allocation and complexity cost.